MDEV-40023 Clean up dead code in check_expected_crash_and_restart#5229
MDEV-40023 Clean up dead code in check_expected_crash_and_restart#5229FarihaIS wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the restart command handling in the MariaDB test suite by removing the unused restart_bindir command and its associated environment variable handling. It also introduces strict error checking for unknown commands in the expect file. The review feedback highlights that the regular expression /^restart/ is too loose and will match any command starting with "restart" (e.g., "restarting"), suggesting it be tightened to /^restart\s*$/ to prevent silently ignoring invalid commands.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
Remove the unused restart_bindir handler from mariadb-test-run.pl and start_mysqld.inc (no test ever sets $restart_bindir). Error on unrecognized expect file commands instead of silently ignoring them. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
LGTM. Please stand by for the final review.
There was a problem hiding this comment.
Pull request overview
Cleans up the .expect-file crash/restart control path in MTR by removing unused restart_bindir support and surfacing invalid expect-file commands as errors, to avoid silent misbehavior during crash/restart tests.
Changes:
- Remove the
restart_bindirhandling fromcheck_expected_crash_and_restart()and fromstart_mysqld.inc. - Treat unrecognized expect-file commands as fatal via
mtr_error()(instead of silently ignoring them). - Skip empty expect-file lines to mitigate partial-write races.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mysql-test/mariadb-test-run.pl | Removes restart_bindir logic; adds stricter parsing/erroring for expect-file commands; skips empty lines. |
| mysql-test/include/start_mysqld.inc | Removes generation of restart_bindir commands in expect files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Anything other than 'wait' or 'restart' will result in an error. | ||
| if ($last_line =~ /^restart:(.+)/) { | ||
| my @rest_opt= split(' ', $1); | ||
| $mysqld->{'restart_opts'}= \@rest_opt; | ||
| } else { | ||
| } elsif ($last_line =~ /^restart/) { |
|
This was done for MDEV-25004 and was used to test upgrade between two build trees. I don't think removing such feature unused in main mtr workflow is a good idea. I've used it in the process of development and may use it later in such projects as MDEV-16417. It is useful for any development when you have to check how the binary data migrates the versions. |
Description
check_expected_crash_and_restart()inmariadb-test-run.plcontains dead code forrestart_bindir(no test ever sets$restart_bindir) and silently ignores unknown commands in the expect file. This made it easy for bugs like MDEV-39153 to go undetected.Fix:
restart_bindirhandler frommariadb-test-run.plandstart_mysqld.incmtr_error()when an unrecognized command is found in the expect fileRelease Notes
N/A
How can this PR be tested?
Execute the full MTR test suite — no test uses
restart_bindir, so removing it should have no effect. Themtr_error()change will prevent any future invalid expect file commands from failing silently.Basing the PR against the correct MariaDB version
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.